-
Notifications
You must be signed in to change notification settings - Fork 23
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Initial Conversion of SCSS to CSS #5375
Conversation
🦋 Changeset detectedLatest commit: 56d9708 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left a couple of comment about some opportunities we have to clean up the variable usage in the CSS.
With the ButtonGroup I can see the a CSS module but a SCSS module deletion in the PR diff / an update to the CSS import.
Ooo, also I'd checkout the UI tests for the Avatar, a little bit of funkiness see to be happening in the Avatar stickersheets 😉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me know if you want to chat through any comments, or if you think any of it is too much work :) Most comments are suggestions for how to compose the CSS better (figured it's a good learning opportunity), but I'm open to descoping some of it if you have other priorities, since this is an improvement regardless.
border: 3px solid var(--color-white); | ||
} | ||
|
||
.wrapper.small { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haha oh no! I must not have been clear enough. Here's an example:
.wrapper.small { | |
.small { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😭
.reversed { | ||
--badge-background-color: rgba(var(--color-white-rgb), 0.1); | ||
|
||
color: var(--color-white); | ||
} | ||
|
||
.reversed.active { | ||
--badge-background-color: var(--color-green-300); | ||
|
||
color: var(--color-purple-800); | ||
} | ||
|
||
.reversed.dark { | ||
--badge-background-color: var(--color-purple-700); | ||
|
||
color: var(--color-white); | ||
} | ||
} | ||
|
||
.badge.large { | ||
display: inline-flex; | ||
justify-content: center; | ||
border-radius: var(--spacing-48); | ||
font-size: var(--typography-data-medium-font-size); | ||
line-height: var(--typography-data-medium-line-height); | ||
letter-spacing: var(--typography-data-medium-letter-spacing); | ||
padding: 2px 1.875rem; | ||
width: 24px; | ||
} | ||
|
||
.default { | ||
color: var(--color-purple-800); | ||
} | ||
|
||
.active { | ||
--badge-background-color: var(--color-blue-500); | ||
|
||
color: var(--color-white); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hopefully this helps a little in understanding the structure?
.reversed { | |
--badge-background-color: rgba(var(--color-white-rgb), 0.1); | |
color: var(--color-white); | |
} | |
.reversed.active { | |
--badge-background-color: var(--color-green-300); | |
color: var(--color-purple-800); | |
} | |
.reversed.dark { | |
--badge-background-color: var(--color-purple-700); | |
color: var(--color-white); | |
} | |
} | |
.badge.large { | |
display: inline-flex; | |
justify-content: center; | |
border-radius: var(--spacing-48); | |
font-size: var(--typography-data-medium-font-size); | |
line-height: var(--typography-data-medium-line-height); | |
letter-spacing: var(--typography-data-medium-letter-spacing); | |
padding: 2px 1.875rem; | |
width: 24px; | |
} | |
.default { | |
color: var(--color-purple-800); | |
} | |
.active { | |
--badge-background-color: var(--color-blue-500); | |
color: var(--color-white); | |
} | |
} | |
.reversed { | |
--badge-background-color: rgba(var(--color-white-rgb), 0.1); | |
color: var(--color-white); | |
} | |
// TODO: .dark.reversed moved into .dark | |
.large { | |
display: inline-flex; | |
justify-content: center; | |
border-radius: var(--spacing-48); | |
font-size: var(--typography-data-medium-font-size); | |
line-height: var(--typography-data-medium-line-height); | |
letter-spacing: var(--typography-data-medium-letter-spacing); | |
padding: 2px 1.875rem; | |
width: 24px; | |
// TODO: | |
// &.dot { ... } can probably go here, but it does mean that .dot must go above it. | |
// this .large can likely go further down in the file after the variants | |
} | |
.default { | |
color: var(--color-purple-800); | |
} | |
.active { | |
--badge-background-color: var(--color-blue-500); | |
color: var(--color-white); | |
&.reversed { | |
--badge-background-color: var(--color-green-300); | |
color: var(--color-purple-800); | |
} | |
} |
text-align: center; | ||
background-color: var(--badge-background-color, var(--color-gray-300)); | ||
|
||
.reversed { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to what we were talking about in Avatar. Nesting the class like this will target the children of the element with the badge class:
Whereas the original SCSS reversed class is at the root the badge stylesheet:
.
A handy thing to lean on here is hovering over the class name in VSCode within the stylesheet, which will bring up the prompt that shows how that class will target elements on the page
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -0,0 +1,48 @@ | |||
.buttonGroup { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well've also need to update ButtonGroup to use this and remove the old SCSS module for buttonGroup
transform: scale3d(1.35, 1.35, 1.35); | ||
} | ||
|
||
.animationOn .badge .dark { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should have the badge and dark together.
At the moment this is looking for a element that is a child of .badge
that has the class name .dark
Whereas before this was looking for a .badge
that also have the class name .dark
:
If you see no space between the &
and .class
, then we can assume that this is specifying both classes :)
font-family: var(--typography-heading-5-font-family); | ||
font-weight: var(--typography-heading-5-font-weight); | ||
letter-spacing: var(--typography-heading-5-letter-spacing); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small optimisation and reduction in code is to move these styles (ln47 - 49) into the base avatarCounter class since they are pretty much the base styles for the counter.
Because the large styles is defiend at the bottom of the doc and have a higher specificity then they should win out if its a large avatar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Merge on good buddy
Why
Converting SCSS to CSS
https://cultureamp.atlassian.net/browse/KZN-2877
What
Converts Avatar, AvatarGroup, Badge, Brand, BrandMoment to CSS